-
Notifications
You must be signed in to change notification settings - Fork 849
Add new API TSContScheduleOnEntirePool and TSContScheduleEveryOnEntirePool #10742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new API TSContScheduleOnEntirePool and TSContScheduleEveryOnEntirePool #10742
Conversation
d7b46aa to
6ce4110
Compare
|
[approve ci] |
1 similar comment
|
[approve ci] |
e682d6f to
b1eee60
Compare
bc44306 to
81ff8b3
Compare
81ff8b3 to
9336092
Compare
src/api/InkAPI.cc
Outdated
| } | ||
|
|
||
| std::vector<tsapi::c::TSAction> | ||
| tsapi::c::TSContScheduleOnEntirePool(TSCont contp, TSHRTime timeout, TSThreadPool tp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does something in the C API return a std::vector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bneradt said it's all c++ api now. Or are you talking about the tsapi::c part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tsapi::c part.
SolidWallOfCode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the double vector implementation. I think it would be reasonable to have the core methods return TSAction instead of TSEvent, because
- The latter can be extracted from the former.
- It avoids the double allocation.
- It hides the pointer hacking in the Event system instead of the API implementation.
That does sound better, I'll change that. Did it this way since the other ones had that part in the API. |
9336092 to
c91ba83
Compare
| is effective until the continuation :arg:`contp` is being dispatched. However, if it is | ||
| scheduled on another thread this can be problematic to be correctly timed. The return value | ||
| can be checked with :func:`TSActionDone` to see if the continuation ran before the return, | ||
| which is possible if :arg:`timeout` is `0`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You refer to this as "timeout" here in the doc, but in the prototype it's "every". That seems inconsistent. Maybe it should be "interval" ?
Also, since you added an explicit "call once" API, what does a "every" of 0 mean ?? Should we even allow that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I copied some of the text from the other docs , every == 0 is not allowed for any of the TSContScheduleEveryXXX calls. I'll fix this.
| #include <ts/ts.h> | ||
| .. function:: std::vector<TSAction> TSContScheduleOnEntirePool(TSCont contp, TSHRTime timeout, TSThreadPool tp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timeout seems wrong name here. It's a delay, right ? I got confused here because the "every" version also uses both timeout and delay "randomly"?
Are these API prototypes the same as what was sent to the mailing list? If not, maybe an update there as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just following the naming convention for the other TSContSchedule APIs. I'll update the email in the mailing list to show the changes of this PR.
The timeout is kind of like a delay, but it could also be a negative number for negative queue events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we got rid of the negative queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty sure it's still here?
| process_queue(&NegativeQueue, &ev_count, &nq_count); |
9683305 to
291ee15
Compare
|
@zwoop I think this is ready for another review |
|
[approve ci format] |
291ee15 to
d277f6a
Compare
d277f6a to
c527966
Compare
c527966 to
6c5e1e0
Compare
|
[approve ci autest] |
Adds two new APIs that allow scheduling continuations on all threads of a certain thread type (e.g. ET_NET, ET_TASK):
The behavior is similar to
EventProcessor::schedule_spawn().